Improved verbose messages in rolling functions frolladaptivefun and frollfun by Omartech312 · Pull Request #7733 · Rdatatable/data.table

Omartech312 · 2026-04-30T00:43:12Z

Closes #7021.

Previously, the frolladaptivefun and rfollfun algo verbose messages were not super user friendly, displaying messages like:

frolladaptivefun: algo 0 not implemented, fall back to 1

The wanted adjustment was for it to instead produce:

frolladaptivefun: algo fast not implemented, fall back to exact

There was also:
frollfun: processing fun 0 algo 0 took...

and the desired result was instead:
frollfun: processing fun mean algo fast took...

We adjusted for the desired functionality by creating an array of strings called rfunNames, using the rfun enum as an index to select the proper display type for the running function and then displaying fast or exact dependent on the value of value (0 for fast, 1 for exact).

Here is an example of the current output (from running tests) with the changed files in this PR:
frolladaptivefun: processing fun MIN algo exact took 0.000s
frolladaptivefun: algo fast not implemented, fall back to exact

We have additionally adjusted the NEWS.md file and the corresponding tests in inst/tests/froll.Rraw.

Updating Project Fork

codecov · 2026-04-30T00:51:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.04%. Comparing base (da0d15e) to head (a51eb5d).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7733   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files          87       87           
  Lines       17058    17064    +6     
=======================================
+ Hits        16895    16901    +6     
  Misses        163      163

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jangorecki

Approach is good, just some comments to make it cleaner.

jangorecki · 2026-04-30T02:17:55Z

+  "frollfun: processing fun MEAN algo fast took.*",
  "frollfunR: processing.*took.*"
-), warning="has.nf=FALSE used but non-finite values are present in input, use default has.nf=NA to avoid this warning")
+), warning="has.nf=FALSE used but non-finite values are present in input, use default has.nf=NA to avoid this warning") #7021


Why is this line changed?

jangorecki · 2026-04-30T02:19:04Z

+  "frollfun: processing fun MEAN algo fast took.*",
  "frollfunR: processing.*took.*"
-))
+)) #7021


I think we can safely skip issue markers in already existing tests

jangorecki · 2026-04-30T02:22:07Z

-  if (verbose)
+  char rfunStr[7];   
+
+  const char *rfunNames[] = {


I think it make sense to define it once at compile time rather than inside frollrun function at runtime.

jangorecki · 2026-04-30T02:23:58Z

+  char rfunStr[7];   
+
+  // enum ordered based from the file src/data.table.h
+  const char *rfunNames[] = {


Same here, if we will move it to data.table.h then we need to only declare it once

jangorecki · 2026-04-30T02:25:38Z

-  if (verbose)
-    snprintf(end(ans->message[0]), 500, _("%s: processing fun %d algo %u took %.3fs\n"), __func__, rfun, algo, omp_get_wtime()-tic);
+  if (verbose) {
+    if(algo == 0)


I would go with ternary if operator, will be much smaller change

jangorecki · 2026-04-30T02:27:51Z

      //frolladaptivemaxFast(x, nx, ans, k, fill, narm, hasnf, verbose); // frolladaptivemaxFast does not exists as of now
-      snprintf(end(ans->message[0]), 500, _("%s: algo %u not implemented, fall back to %u\n"), __func__, algo, (unsigned int) 1);
+      snprintf(end(ans->message[0]), 500, _("%s: algo fast not implemented, fall back to exact\n"), __func__);
+      algo = 1;


Is this line needed/used? It wasn't there before, if it is not needed/used, then we could still have it, but then better commented out with a comment that it is not used.

This line is currently necessary because some of the fast functions are not implemented yet. In those cases, even if "fast" was requested, the code falls back to the exact version, so this makes sure the final message prints the algorithm that was actually used.

jangorecki · 2026-04-30T02:28:02Z

      //frolladaptiveminFast(x, nx, ans, k, fill, narm, hasnf, verbose); // frolladaptiveminFast does not exists as of now
-      snprintf(end(ans->message[0]), 500, _("%s: algo %u not implemented, fall back to %u\n"), __func__, algo, (unsigned int) 1);
+      snprintf(end(ans->message[0]), 500, _("%s: algo fast not implemented, fall back to exact\n"), __func__);
+      algo = 1;


Fast -> Exact message

jangorecki · 2026-04-30T02:28:28Z

-  if (verbose)
-    snprintf(end(ans->message[0]), 500, _("%s: processing fun %d algo %u took %.3fs\n"), __func__, rfun, algo, omp_get_wtime()-tic);
+  if (verbose) {
+    if(algo == 0)


jangorecki · 2026-04-30T02:28:57Z

  }
  free(xx);
-}
+}


This change should be reverted

jangorecki · 2026-04-30T02:30:12Z


 5. `tables()` can now optionally report `data.table` objects stored one level deep inside list objects when `depth=1L`, [#2606](https://github.com/Rdatatable/data.table/issues/2606). Thanks @MichaelChirico for the report and @manmita for the PR

+<<<<<<< HEAD


Move new entry below existing ones and clean code markers (<<<, ===, >>>) after git conflict.

jangorecki · 2026-05-03T18:19:28Z

  "frollfun: processing fun MEAN algo fast took.*",
  "frollfunR: processing.*took.*"
-)) #7021
+))  


White spaces are not needed either

jangorecki · 2026-05-03T18:25:24Z

 5. `tables()` can now optionally report `data.table` objects stored one level deep inside list objects when `depth=1L`, [#2606](https://github.com/Rdatatable/data.table/issues/2606). Thanks @MichaelChirico for the report and @manmita for the PR
-6. 
-7. `yearqtr()` and `yearmon()` now gain an optional format specifier [#7694](https://github.com/Rdatatable/data.table/issues/7694). 'numeric' is the default, which preserves the original behavior, but 'character' formats `yearqtr()` as YYYYQ# (e.g. 2025Q2) and `yearmon()` as YYYYM## (e.g. 2025M02, 2025M10). Thanks to @jan-swissre for the report and @LunaticSage218 for the implementation.
+   


Please avoid white spaces only lines, similarly as trailing white spaces

… (defined in froll.c & extern in data.table.h)

aidengseay and others added 4 commits April 29, 2026 15:46

Merge pull request #1 from Rdatatable/master

be1b189

Updating Project Fork

Adding updated files

5d80b2b

Updated NEWS.md

5239716

Updated test file

f033f91

Omartech312 requested review from MichaelChirico and jangorecki as code owners April 30, 2026 00:43

jangorecki reviewed Apr 30, 2026

View reviewed changes

kkarissa and others added 5 commits May 1, 2026 18:30

Fix test warnings and update output messages

09420a9

Resolve merge conflict in NEWS.md

b8ae8e2

Update NEWS.md with new features and fixes

68a00f9

Change to ternary if operator

47e4131

Change to ternary if operator in froll.c

1c84904

jangorecki reviewed May 3, 2026

View reviewed changes

Omartech312 added 7 commits May 3, 2026 11:28

Removing white space in froll.Rraw

52fb2e8

Removing whitespace from NEWS.md

1a167e4

Refactoring rfunNames for copile time definition (in data.table.h)

6d1a9a6

Removing extra brackets

dc621d7

Declare extern array for roll function names

077e638

Reverting changes for rfunStr & adjusting for direct use of rfunNames…

c41127a

… (defined in froll.c & extern in data.table.h)

Update comments for clarity in frolladaptive.c

a51eb5d


		5. `tables()` can now optionally report `data.table` objects stored one level deep inside list objects when `depth=1L`, [#2606](https://github.com/Rdatatable/data.table/issues/2606). Thanks @MichaelChirico for the report and @manmita for the PR

		<<<<<<< HEAD

Conversation

Omartech312 commented Apr 30, 2026

Uh oh!

codecov Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jangorecki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Apr 30, 2026 •

edited

Loading